Skip to content

[Mobile Payments] Fix Tap to Pay on iPhone setup failures on second account #8633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 18, 2023
Merged

[Mobile Payments] Fix Tap to Pay on iPhone setup failures on second account #8633

merged 2 commits into from
Jan 18, 2023

Conversation

@joshheald
Copy link
Contributor

@joshheald joshheald commented Jan 13, 2023 •
edited
Loading

Part of: #8290

Description

When logging out of an account after using IPP, then logging in to a second account, The Tap to Pay on iPhone connection flow was always failing, and entering a loop.

This PR prevents the flow from always failing, but does not yet fix the loop, which can still be seen if you cancel the connection flow.

Cause of the failures: not updating the tokenProvider

The cause of this bug is that in the attempts on the second account, we're using an old tokenProvider in the Terminal, so the locationIDs we get are being used against the wrong Stripe account (unconfirmed, but at least with the wrong token.)

We correctly call Terminal.clearCachedCredentials when the store is switched and when the user logs out. That means that the Terminal calls the original tokenProvider (which it still has a reference to) to get a new token when we next connect to a reader.

We can't set a new tokenProvider, because the app crashes with this exception from Stripe:

[StripeTerminal] ⚠️ An integration error was detected in your app. You can only call setTokenProvider before requesting the shared Terminal instance for the first time. We recommend calling setTokenProvider in your AppDelegate's application:didFinishLaunchingWithOptions: method.

Fix

To fix it, I've put the token provider on the ServiceLocator, so that we keep hold of it for the lifetime of the app, and then the existing code will update the token that it vends, and the provider that it uses.

Concerns

This is likely just one presentation of this bug, and the fix will affect the existing bluetooth reader flow. We need to test these flows carefully.

Testing instructions

For the original bug

  1. Launch the app fresh, and log in to an account with a US WCPay store, selecting that store
  2. Navigate to Menu > Settings > Experimental features
  3. Turn on Tap to Pay on iPhone
  4. Navigate to Menu > Payments > Collect payment
  5. Go through the payment flow, and select Card on the payment method screen
  6. When asked for a reader type, tap Tap to Pay on iPhone and go through the Terms of Service Apple ID linking (if you've not done so before)
  7. Take the payment, then log out
  8. Log in to a different account, which has another store that has not had the ToS linking process done
  9. Repeat the above steps to take a payment
  10. When you get to the ToS Apple ID linking step, observe that you can complete it and then take a payment.

Connecting a Bluetooth reader

  1. Launch the app fresh, and log in to an account with a US WCPay store, selecting that store
  2. Navigate to Menu > Settings > Experimental features
  3. Turn off Tap to Pay on iPhone
  4. Navigate to Menu > Payments > Collect payment
  5. Go through the payment flow, and select Card on the payment method screen
  6. Connect to a Bluetooth reader: Chipper, M2, or WisePad3
  7. Take the payment, then log out
  8. Log in to a different account, which has another store
  9. Repeat the above steps to take a payment
  10. Observe that you can connect to a reader and then take a payment. (On trunk, you can't connect.)

Screenshots

ttpoi-account-linking-fix.mp4
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@joshheald
8290 Update tests to simplify adding dependency
434bfdd
@joshheald
8290 Use one ReaderConfigProvider for the app’s lifetime
009a740 
Once the CardReaderConfigProvider is set on the Terminal, it can’t be changed. This means we need to keep the same one around for the lifetime of the app in order to change it when the user logs out and back in again.

Failing to do this results in serious errors when using a second account to take IPP transactions.
@joshheald joshheald added type: bug A confirmed bug. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. labels Jan 13, 2023
@joshheald joshheald added this to the 12.0 milestone Jan 13, 2023
@joshheald joshheald changed the base branch from trunk to issue/8089-disconnection-error-message January 13, 2023 17:37
@joshheald joshheald requested a review from lmischner January 13, 2023 17:54
@joshheald
Copy link
Contributor Author

joshheald commented Jan 13, 2023

@lmischner Here's a tentative fix for part of the ToS acceptance bug. It doesn't address what happens if you cancel the ToS flow, I'll do that next week, but it does fix the happy path.

It's quite a high impact fix though, so it would be great if you have some time to do exploratory testing around it, particularly covering the bluetooth reader flows, and flows where the ToS acceptance is already done. I will also do some more of this next week.

Thanks!

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 13, 2023

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8633-009a740 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@joshheald joshheald marked this pull request as ready for review January 16, 2023 10:38
@joshheald
Copy link
Contributor Author

joshheald commented Jan 16, 2023

@lmischner I've found the Bluetooth flavour of this issue in the production app, and added testing instructions for that.

On trunk, when you try to connect to the Bluetooth reader in the second account you'll get this error:

bluetooth-connection-bug-switching-accounts.mp4

N.B. These repro steps don't seem to quite be 100%, and I'm not sure why it doesn't always fail, but I've tested the fix enough to have confidence in it.

Based on the fix, the same can probably be done switching between reader types too, but all are fixed by this PR.

@joshheald joshheald requested a review from toupper January 16, 2023 11:13
@joshheald
Copy link
Contributor Author

joshheald commented Jan 16, 2023

@toupper Just to note that this fix is in the production code, not just the feature flagged TTPoI bit. It also fixes the bug in production as you can see from comments above.

I've moved the CommonReaderConfigProvider to the ServiceLocator, so it's explicitly handled as a singleton, just like the CardReaderService is.

Up to now it's treated as a singleton by StripeTerminal, which gets and keeps a reference to it when first used. However, we didn't keep a reference to it, so we couldn't effectively update it for a new user login.

I considered just keeping it on the CardReaderService, as a let property, but it's Yosemite which needs to update it, so left it in there as its own Singleton. Let me know if you have any thoughts on the way it's done.

@toupper
Copy link
Contributor

toupper commented Jan 16, 2023

@joshheald The code looks good to me ✅ I didn't test it as the setup would take a while, so I will rely on @lmischner's and your testing. Great job!

Base automatically changed from issue/8089-disconnection-error-message to trunk January 16, 2023 17:28
@lmischner
Copy link
Contributor

lmischner commented Jan 17, 2023

If I connect to the bluetooth reader in one account and then log out/login to another account, should the connection be maintained?

Otherwise, no questions/issues found!

@joshheald
Copy link
Contributor Author

joshheald commented Jan 18, 2023

If I connect to the bluetooth reader in one account and then log out/login to another account, should the connection be maintained?

Thanks for the review @lmischner – no, the connection should not be maintained in this case. When we connect to a reader we specify the (Stripe) account we're connecting for, which is unique to the store. So whenever we change account or store, we'll need to reconnect to the reader. In my testing, that's maintained, but sorry that I didn't call it out in the testing notes.

@joshheald joshheald merged commit 5d5677d into trunk Jan 18, 2023
@joshheald joshheald deleted the issue/8290-ttpoi-setup-connection-loop branch January 18, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toupper toupper toupper approved these changes

@lmischner lmischner Awaiting requested review from lmischner

Assignees

No one assigned

Labels

feature: mobile payments Related to mobile payments / card present payments / Woo Payments. type: bug A confirmed bug.

Projects

None yet

Milestone

12.0

Development

Successfully merging this pull request may close these issues.

5 participants

@joshheald @wpmobilebot @toupper @lmischner